Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add test for Sse method indexing on native #1613

Merged
merged 3 commits into from
Jan 16, 2024
Merged

Add test for Sse method indexing on native #1613

merged 3 commits into from
Jan 16, 2024

Conversation

mocenas
Copy link
Contributor

@mocenas mocenas commented Jan 15, 2024

Summary

Add tests for quarkusio/quarkus#36986

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@mocenas mocenas requested a review from jedla97 January 15, 2024 14:09
@mocenas
Copy link
Contributor Author

mocenas commented Jan 15, 2024

run tests

fedinskiy
fedinskiy approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@fedinskiy fedinskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you probably want to squash these two commits together

jedla97
jedla97 reviewed Jan 15, 2024
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mocenas I looking at it and I thing it is covered by #1517 as I disable it here https://github.com/quarkus-qe/quarkus-test-suite/blob/main/http/http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/BaseHttpAdvancedReactiveIT.java#L453
So I don't know if we want to merge almost same code except you seem to have little different updates method. As this is both in http reactive part of TS. Can you look at it if it's test different thing or it's the same?

Btw in that PR it was updated to not use only String as StringBuilder is not thread safe and LockSupport was replaced by CountDownLatch

@mocenas
Copy link
Contributor Author

mocenas commented Jan 16, 2024

@mocenas I looking at it and I thing it is covered by #1517 as I disable it here https://github.com/quarkus-qe/quarkus-test-suite/blob/main/http/http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/BaseHttpAdvancedReactiveIT.java#L453 So I don't know if we want to merge almost same code except you seem to have little different updates method. As this is both in http reactive part of TS. Can you look at it if it's test different thing or it's the same?

Btw in that PR it was updated to not use only String as StringBuilder is not thread safe and LockSupport was replaced by CountDownLatch

I'm unable to reproduce the issue quarkusio/quarkus#36986 with this test. I was not able to reproduce the issue in the module "http-advanced-reactive" at all. That's why I put it in rest-client-reactive module. I assume it is because it uses "jackson" version of rest client, but I'm not sure.

I tried to reproduce it with quarkus-bom v 3.5.1. If you have better luck reproducing it, I agree it would be better to not duplicate this, otherwise we will have to.

I agree that using thread safe things is better.

@mocenas
Copy link
Contributor Author

mocenas commented Jan 16, 2024

@jedla97 Please review ^^ if it is possible to reproduce this issue with current code, or should we merge this PR.

jedla97
jedla97 requested changes Jan 16, 2024
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mocenas thanks for update and change it to thread safe. Just one small thing can you squash the commits to one.

Tested it and I was unable to reproduce the error (still don't know why). So yes we need to merge this. Thanks for looking at it and point it to me.

@mocenas
Copy link
Contributor Author

mocenas commented Jan 16, 2024

I will squash the commits on github merge.

@mocenas mocenas merged commit 834e109 into quarkus-qe:main Jan 16, 2024
7 checks passed
@jedla97
Copy link
Member

jedla97 commented Jan 16, 2024

OK. I know it exist but wasn't sure how the result will look. Now I know 😄

@mocenas mocenas deleted the test_sse_NoSuchMethodException branch January 16, 2024 14:06
@mocenas mocenas added the triage/backport-3.2 RHBQ Ghost LTS release label Jan 17, 2024
michalvavrik pushed a commit to michalvavrik/quarkus-test-suite that referenced this pull request Jan 22, 2024
@mocenas @michalvavrik
Add test for Sse method indexing on native (quarkus-qe#1613)
200c39b 
* Add test for Sse method indexing on native
@michalvavrik michalvavrik mentioned this pull request Jan 22, 2024
Merged
9 tasks
@michalvavrik michalvavrik removed the triage/backport-3.2 RHBQ Ghost LTS release label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedla97 jedla97 jedla97 requested changes

@fedinskiy fedinskiy fedinskiy approved these changes

Assignees

@fedinskiy fedinskiy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mocenas @jedla97 @fedinskiy @michalvavrik